Support specific aliases for PnP step labels #3461
Conversation
📝 WalkthroughWalkthroughThe Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/pnp/findStepColumns.ts (1)
77-79: Remove commented-out code.The commented-out code appears to be unused and should be removed to maintain code cleanliness.
Apply this diff to remove the commented code:
- // const isApproved = ApprovedCustomSteps.get( - // label.trim().toLowerCase(), - // );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/pnp/findStepColumns.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: Clean
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: lint
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Unit
🔇 Additional comments (1)
src/components/pnp/findStepColumns.ts (1)
48-54: LGTM! Correct normalization of sheet labels.The normalization logic for sheet labels is implemented correctly - it properly converts custom string headers from the spreadsheet to standard Step enum values before fuzzy matching.
rdonigian
force-pushed
the
step-header-mapping
branch
2 times, most recently
from
36f84c9 to
79984d3
Compare
- Map better represents key optionality - Set lets us not use lodash without - chooseStep moves that BL to a pure function
These should be matched exactly and not go through the fuzzy matching
If there are two columns with "first draft" then the second will become a non-standard step problem.
CarsonF
force-pushed
the
step-header-mapping
branch
from
79984d3 to
b83b214
Compare
CarsonF
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/product-progress/step-progress-extractor.service.ts (1)
81-88: 💡 Verification agent🛠️ Refactor suggestion
🧩 Analysis chain
!on.get()risks runtime crash when a column is missing
planningStepColumns.get(step)!will throw if the step wasn’t found in the planning sheet (e.g. header typo, alias not mapped, or a step removed later).
Gracefully skip (or log a problem) instead of crashing the extraction.- const fiscalYear = pnp.planning.cell( - planningStepColumns.get(step)!, - planningRow, - ); + const planCol = planningStepColumns.get(step); + if (!planCol) { + // Column not present – record and skip instead of crashing. + result.addProblem?.('MissingPlanningStepColumn', cell, { step }); + return []; + } + const fiscalYear = pnp.planning.cell(planCol, planningRow);
🏁 Script executed:
#!/bin/bash rg "planningStepColumns" -n src/components/product-progress/step-progress-extractor.service.tsLength of output: 347
🏁 Script executed:
#!/bin/bash # Locate and display the parseProgressRow function to understand its callback context rg -n "parseProgressRow" -A20 -B10 src/components/product-progress/step-progress-extractor.service.tsLength of output: 1329
🏁 Script executed:
#!/bin/bash # Display the full parseProgressRow callback to see its return structure sed -n '67,140p' src/components/product-progress/step-progress-extractor.service.ts # Find the definition of PnpProgressExtractionResult to verify addProblem exists rg -n "PnpProgressExtractionResult" -A5 -B5 -n srcLength of output: 15751
Handle missing planningStepColumns entry to prevent runtime crash
A non-null assertion on
planningStepColumns.get(step)!will throw if that step isn’t mapped in the planning sheet. We should guard against a missing column, log the issue, and skip the step.• File:
src/components/product-progress/step-progress-extractor.service.ts
• Location: inside theparseProgressRowcallback whereentries(stepColumns).flatMapis used (around lines 81–88)Suggested diff:
const steps = entries(stepColumns).flatMap<StepProgressInput>( ([step, column]) => { - const fiscalYear = pnp.planning.cell( - planningStepColumns.get(step)!, - planningRow, - ); + const planCol = planningStepColumns.get(step); + if (!planCol) { + // Missing planning sheet column for this step – record and skip. + const progCell = sheet.cell(column, row); + result.addProblem('MissingPlanningStepColumn', progCell, { step }); + return []; + } + const fiscalYear = pnp.planning.cell(planCol, planningRow); const cell = sheet.cell(column, row); if ( !isGoalStepPlannedInsideProject(pnp, fiscalYear, step, result) || isProgressCompletedOutsideProject(pnp, cell, step, result) ) { return []; } return { step, completed: progress(cell) }; }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/pnp/findStepColumns.ts(2 hunks)src/components/product-progress/step-progress-extractor.service.ts(2 hunks)src/components/product/product.extractor.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/pnp/findStepColumns.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/product/product.extractor.ts (1)
src/common/xlsx.util.ts (1)
Column(373-409)
src/components/product-progress/step-progress-extractor.service.ts (1)
src/common/xlsx.util.ts (1)
Column(373-409)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: lint
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: Clean
- GitHub Check: Unit
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/components/product-progress/step-progress-extractor.service.ts (1)
69-71: Immutable maps 👍Switching to
ReadonlyMapis a solid improvement—prevents accidental mutation and makes intent explicit.src/components/product/product.extractor.ts (1)
83-86: Consistent immutability 👍The parameter type change to
ReadonlyMapmirrors the progress extractor and keeps data structures consistent across services.
|
@rdonigian want to review my changes? |
This will remedy the custom step header error for a leadership approved list of alternative headers by replacing custom steps with standard ones.